Add a --react-native-package option to vendor-hermes command
#287
Conversation
kraenhansen
added
the
Apple 🍎
|
@shirakaba I'd love your feedback on this approach. |
There was a problem hiding this comment.
So to my understanding, this would mean that RNM users can obtain a suitable value for REACT_NATIVE_OVERRIDE_HERMES_DIR (which they could then set in their CLI, or just write into their Podfile) with:
npx react-native-node-api vendor-hermes --silent --react-native-package react-native-macos(your example said --react-native-package react-native – correct me if I'm wrong to use --react-native-package react-native-macos!)
I think it's certainly a step forward and makes it possible to proceed, so as it stands currently, it's worth merging already.
Though in terms of an ideal implementation, my impression of this approach is that it treats react-native-macos as a second-class citizen. RNM already has a thousand extra steps to get set up correctly and it would be nice to not have another gotcha along the way for Node-API modules, especially given that this concerns CocoaPods installs, which are very time-consuming.
I wonder if there's any way we can figure out that the script is being called from a react-native-macos app's Podfile. Like, inside packages/host/scripts/patch-hermes.rb, maybe if we're lucky there's some environment variable that will already have been set by the Podfile that we could use to automatically determine that we're running in a react-native-macos app, and if so, pass --react-native-package react-native-macos along to the vendor-hermes script.
| else | ||
| raise "Hermes patching failed. Please check the output above for errors." | ||
| if ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].nil? | ||
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip |
There was a problem hiding this comment.
Is there any reason for this to be ||= rather than =? As VENDORED_HERMES_DIR is a local variable, I think it can only be unset at this point?
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip | |
| VENDORED_HERMES_DIR = `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip |
There was a problem hiding this comment.
It's my understanding that Ruby scripts aren't caching requires the same way Node.js would.
I had this originally because it prevents re-execution of the command if the .rb file is (transitively) "required" multiple times.
| if ENV['REACT_NATIVE_OVERRIDE_HERMES_DIR'].nil? | ||
| VENDORED_HERMES_DIR ||= `npx react-native-node-api vendor-hermes --silent '#{Pod::Config.instance.installation_root}'`.strip |
There was a problem hiding this comment.
Summary of the proposed behaviour for my benefit:
- If
ENV.REACT_NATIVE_OVERRIDE_HERMES_DIRis defined:- The env var is used as-is.
- The script checks whether the directory is exists.
- If it exists, then good to go.
- If not, an exception is raised.
- If
ENV.REACT_NATIVE_OVERRIDE_HERMES_DIRis not definednpx react-native-node-api vendor-hermes --silentis called, ultimately vendoring Hermes forreact-native(regardless of whether the user is usingreact-nativeorreact-native-macos.- That vendored directory gets set as the value for
ENV.REACT_NATIVE_OVERRIDE_HERMES_DIR.
There was a problem hiding this comment.
Exactly and I agree it would be ideal if we didn't vendor for react-native if this was driven by a MacOS app. Let's make that happen in a follow-up PR, ideally when we actually have a MacOS app in the repo, testing that flow.
You're right! I updated the description 👍 My bad.
You're right - once I have a better testing setup, I'd love for us to add calling this conditionally "the right" I think we'd want a MacOS test app in the repo to ensure thats a tested path? |
2 participants
A first attempt at solving #270.
Merging this PR will:
--react-native-packageoption to thevendor-hermescommand, controlling what package name to use when determining the base version of Hermes to use for cloning.sdks/node-api-hermesof the React Native package (choosen via--react-native-package☝️)scripts/patch-hermes.rbto allow supplying aREACT_NATIVE_OVERRIDE_HERMES_DIRfrom the outside to skip callingvendor-hermesinternally.With this, you should be able to clone and override Hermes in the MacOS out-of-tree package of React Native running (in
<your-app>/ios):REACT_NATIVE_OVERRIDE_HERMES_DIR=$(npx react-native-node-api vendor-hermes --silent --react-native-package react-native-macos) pod install